Optimize sequence_gen and uniform_sequence_gen to reduce template instantiation depth #3585
Conversation
tenpercent
force-pushed
the
tenpercent/old-ck-pack-rewrites
branch
from
a477221 to
57c8cb1
Compare
|
Do you want to add unit tests for this, or just rely on the tests of all the code that depends on this? If it's easy to add unit tests, that's generally better, but I'm also fine with moving fast to cut down compilation times. |
| // generate sequence | ||
| template <index_t NSize, typename F> | ||
| struct sequence_gen | ||
| // Four sequences: direct concatenation |
There was a problem hiding this comment.
I like these specializations. It will be interesting to get a survey of the code to see how often the specializations are used and if these four smallest cases are the most impactful ones.
There was a problem hiding this comment.
I'm using the build traces to drive the optimizations. Maybe removing the unused code is one other aspect which could help with parsing times
let's move fast, in case something important breaks CI will catch it, tests also need maintenance and let's see how much of the metaprogramming is left after the initial sprint |
tenpercent
requested review from
Snektron,
ThomasNing,
afagaj,
andriy-ca,
aosewski,
asleepzzz,
bartekxk,
carlushuang,
cgmillette,
coderfeli,
geyyer,
illsilin,
poyenc,
qianfengz,
vidyasagar-amd and
vpietila-amd
as code owners
tenpercent
force-pushed
the
tenpercent/old-ck-pack-rewrites
branch
from
57c8cb1 to
3d46680
Compare
tenpercent
force-pushed
the
tenpercent/old-ck-pack-rewrites
branch
from
02dd8d6 to
3dcda67
Compare
…tiation depth This change significantly improves compile-time performance by reducing template instantiation depth for sequence generation and merging operations: Optimizations: - sequence_gen: Reduce instantiation depth from O(log N) to O(1) by using __make_integer_seq to generate indices in a single step, then applying the functor via pack expansion - uniform_sequence_gen: Similarly optimized to O(1) depth using __make_integer_seq with a helper that applies a constant value via pack expansion - sequence_merge: Reduce depth from O(N) to O(log N) using binary tree reduction strategy. Added direct concatenation specializations for 1-4 sequences to avoid recursion in common cases, falling back to binary tree merging for 5+ sequences Documentation: - Added extensive inline comments explaining why sequence_merge cannot achieve O(1) depth like sequence_gen (requires computing cumulative sequence lengths from heterogeneous inputs, inherently requiring recursion) - Documented the binary tree reduction approach and why it's superior to fold expressions for this use case Testing: - Added comprehensive unit tests for uniform_sequence_gen with different values, sizes, and edge cases - Added tests for sequence_gen with custom functors (double, square, identity, constant) to verify the new implementation works with arbitrary functors - Added tests for sequence_merge with 4, 5, and many sequences to verify both the direct concatenation path and binary tree reduction path - Added tests for empty sequence edge cases
tenpercent
force-pushed
the
tenpercent/old-ck-pack-rewrites
branch
from
3dcda67 to
8826238
Compare
shumway
left a comment
shumway
left a comment
There was a problem hiding this comment.
This looks great. I may be misreading, but it looks like we're sill O(N) for large number of sequences, but that's not important.
My reading is that we keep merging the first pair of seqeunces, not really a binary reduction. But we could merge the first triple of sequences easily with a one-line change.
| struct sequence_merge_impl<S1, S2, S3, S4, Rest...> | ||
| { | ||
| // Merge pairs first, then recurse | ||
| using left = typename sequence_merge_impl<S1, S2>::type; |
There was a problem hiding this comment.
I may be misreading this, but since we have three hard-coded, is faster to use <S1, S2, S3> for left and <S4, Rest...> for right.
Also, my reading is that this is still O(N) when N is a large number of sequences, but we're cutting down the prefactor and hardcoding smaller numbers of sequences.
There was a problem hiding this comment.
yes, good catch! covering ~70% of the instantiations with the base cases is probably the actual impact. we can follow up with it if it shows up in the build traces again
Summary
__make_integer_seqforsequence_genanduniform_sequence_gensequence_mergeusing direct concatenation for small casesMotivation
The recursive
sequence_gen_implcreates deep template chains for large sequences, increasing compile time and memory usage.Why It Works
The compiler builtin
__make_integer_seqgenerates the entire integer sequence in a single step internally, without recursive template instantiation. This reduces instantiation depth from O(N) to O(1), significantly cutting compile time for large sequences.Additionally,
sequence_mergenow uses direct concatenation for 2-3 sequences instead of recursive merge, reducing overhead.Build Performance Impact
Template Instantiation Reduction (measured on
device_grouped_conv3d_fwd_bias_bnorm_clamp_instancetarget, 248 files):This confirms the optimization successfully reduces template instantiation overhead by using compiler builtins.
sequence_merge_impl Instantiation Analysis:
Analyzed 110,795
sequence_merge_implinstantiations across the build target:Current coverage: 73.1% handled by base cases (1-4 sequences)
Opportunity: Adding a 5-sequence base case would increase coverage to 82.1% (+9.0%)
The existing base cases handle the most common patterns efficiently. The 5-sequence case represents the largest remaining optimization opportunity.
Test Plan
sequence_genwith custom functorsPR Stack
This PR is part of the build time optimization effort (issue #3575). All PRs now target develop independently:
__make_integer_seqTracking issue: #3575